Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Make placeholder accessible #6171

Merged
merged 8 commits into from
Jul 4, 2024
Merged

Make placeholder accessible #6171

merged 8 commits into from
Jul 4, 2024

Conversation

zurfyx
Copy link
Member

@zurfyx zurfyx commented May 24, 2024

Placeholder won't read to screen readers, it technically can be read but it is not in the expected position in the flow. Instead, it should use placeholder aria-placeholder on the contenteditable.

Placeholder is now part of the ContentEditable (backward compatible for now), for two reasons:

  1. It makes sense to couple the contenteditable with the placeholder. That's how native placeholder and input work.
  2. This is a common pitfall. By coupling them we can ensure that either these properties are provided or none (when there's no placeholder).

Before, doesn't read, interferes with the flow:

Screen.Recording.2024-05-24.at.3.15.28.PM.mov

After:

Screen.Recording.2024-05-24.at.3.28.28.PM.mov

@vercel Vercel
Copy link

vercel bot commented May 24, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
lexical ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 3, 2024 10:47pm
lexical-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 3, 2024 10:47pm

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 24, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented May 24, 2024 •
edited
Loading

size-limit report 📦

Path Size
lexical - cjs 28.47 KB (0%)
lexical - esm 28.28 KB (0%)
@lexical/rich-text - cjs 36.86 KB (0%)
@lexical/rich-text - esm 28.08 KB (0%)
@lexical/plain-text - cjs 35.49 KB (0%)
@lexical/plain-text - esm 25.3 KB (0%)
@lexical/react - cjs 38.82 KB (0%)
@lexical/react - esm 29.27 KB (0%)

StyleT
StyleT previously approved these changes May 24, 2024
View reviewed changes
@Sahejkm Sahejkm added the extended-tests Run extended e2e tests on a PR label May 24, 2024
Fetz
Fetz previously approved these changes May 24, 2024
View reviewed changes
@zurfyx zurfyx force-pushed the a11yplaceholder branch from e875ecf to d936b56 Compare May 25, 2024 13:53
@etrepum
Copy link
Collaborator

etrepum commented May 25, 2024

I can't repro that integration test failure locally (likely has something to do with the github node cache behavior) but perhaps removing the --no-save --no-package-lock from the suite might fix it
https://github.com/facebook/lexical/blob/main/scripts/__tests__/integration/utils.js#L92

@zurfyx zurfyx dismissed stale reviews from StyleT and Fetz via 4113dac May 27, 2024 19:35
ivailop7
ivailop7 previously approved these changes May 27, 2024
View reviewed changes
potatowagon
potatowagon previously approved these changes May 28, 2024
View reviewed changes
@zurfyx zurfyx force-pushed the a11yplaceholder branch from 4113dac to b93cfee Compare May 28, 2024 21:32
@zurfyx zurfyx dismissed stale reviews from potatowagon and ivailop7 via 86dc980 June 18, 2024 18:07
@zurfyx zurfyx force-pushed the a11yplaceholder branch from b93cfee to 86dc980 Compare June 18, 2024 18:07
@zurfyx zurfyx requested a review from Sahejkm as a code owner June 18, 2024 18:07
potatowagon
potatowagon previously approved these changes Jun 19, 2024
View reviewed changes
@Sahejkm Sahejkm removed the extended-tests Run extended e2e tests on a PR label Jun 19, 2024
@zurfyx zurfyx force-pushed the a11yplaceholder branch from a5e939a to c39cbcb Compare July 3, 2024 20:39
@zurfyx zurfyx added the extended-tests Run extended e2e tests on a PR label Jul 3, 2024
@zurfyx zurfyx added this pull request to the merge queue Jul 4, 2024
Merged via the queue into main with commit ebec987 Jul 4, 2024
39 checks passed
@Sahejkm Sahejkm mentioned this pull request Jul 4, 2024
Merged
2wheeh pushed a commit to 2wheeh/lexical that referenced this pull request Jul 17, 2024
@zurfyx @2wheeh
Make placeholder accessible (facebook#6171)
0393671
@korbit-ai korbit-ai bot mentioned this pull request Aug 15, 2024
Open
@local-dev-korbit-ai-mentor local-dev-korbit-ai-mentor bot mentioned this pull request Aug 15, 2024
Open
@phazingazrael phazingazrael mentioned this pull request Aug 23, 2024
Open
@etrepum etrepum mentioned this pull request Nov 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sahejkm Sahejkm Sahejkm approved these changes

@potatowagon potatowagon potatowagon left review comments

@ivailop7 ivailop7 ivailop7 left review comments

@StyleT StyleT StyleT left review comments

@Fetz Fetz Fetz left review comments

@fantactuka fantactuka Awaiting requested review from fantactuka fantactuka is a code owner

@acywatson acywatson Awaiting requested review from acywatson acywatson is a code owner

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. extended-tests Run extended e2e tests on a PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@zurfyx @etrepum @Fetz @StyleT @ivailop7 @potatowagon @Sahejkm @facebook-github-bot